Skip to content

feat(vue-router): upgrade to vue-router 5#31117

Open
ShaneK wants to merge 12 commits intomajor-9.0from
feat/vue-router-upgrade
Open

feat(vue-router): upgrade to vue-router 5#31117
ShaneK wants to merge 12 commits intomajor-9.0from
feat/vue-router-upgrade

Conversation

@ShaneK
Copy link
Copy Markdown
Member

@ShaneK ShaneK commented May 6, 2026

Issue number: internal


What is the current behavior?

@ionic/vue-router and @ionic/vue build against vue-router 4

What is the new behavior?

Bumps vue-router to ^5.0.6 and vue to ^3.5.0 (vue-router 5 raises its peer to ^3.5.0). We also added Playwright tests for Vue router that are in full parity with the previous Jest and removed the Jest tests and replaced them with Playwright.

This PR also makes the current Vue test app, which is also used for the Vue Router automated tests, get rebuilt with the current PR version for testing in the Vercel preview links.

Does this introduce a breaking change?

  • Yes
  • No

Consumer apps that pin vue-router themselves need to upgrade to ^5.0.0, and apps that explicitly pin vue need to bump to ^3.5.0

Other information

CI changes: CI no longer runs npm run test.spec (the script and Jest devDeps are gone), and now runs playwright tests

Preview (Vue + Vue Router test app, demos both packages from this PR):
https://ionic-framework-git-feat-vue-router-upgrade-ionic1.vercel.app/vue/

@ShaneK ShaneK requested a review from a team as a code owner May 6, 2026 17:47
@ShaneK ShaneK requested a review from gnbm May 6, 2026 17:47
@vercel
Copy link
Copy Markdown

vercel Bot commented May 6, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
ionic-framework Ready Ready Preview, Comment May 7, 2026 9:56pm

Request Review

@github-actions github-actions Bot added package: core @ionic/core package package: vue @ionic/vue package package: react @ionic/react package labels May 6, 2026
@ShaneK ShaneK changed the title Feat/vue router upgrade feat(vue-router): upgrade to vue-router 5 May 6, 2026
@ShaneK ShaneK force-pushed the feat/vue-router-upgrade branch from 561f950 to 3f69ec4 Compare May 6, 2026 18:09
@github-actions github-actions Bot removed the package: react @ionic/react package label May 6, 2026
@reslear
Copy link
Copy Markdown

reslear commented May 6, 2026

Nice, Will it work with vue-router/auto-routes ?

shell: bash
working-directory: ./packages/vue/test/build/${{ inputs.app }}
- name: 📦 Install Playwright Browsers
run: npx playwright install chromium
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why Chromium? The Angular tests don't pass in a browser.

Copy link
Copy Markdown
Member Author

@ShaneK ShaneK May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Angular tests actually still only run Chromium despite downloading all 3 browsers. Both the vue tests and react tests only download the browser they use. It could be worth maybe adding support for running all 3 browsers in the future, but I think most of those issues are caught in core.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's the case, can you either make a new PR or in this one to update the other frameworks to only use chromium. No need to inconsistent.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread packages/vue-router/scripts/test_runner.sh Outdated
Comment thread packages/vue-router/package.json
Comment thread packages/vue/test/apps/vue3/package.json
@ShaneK
Copy link
Copy Markdown
Member Author

ShaneK commented May 7, 2026

Nice, Will it work with vue-router/auto-routes ?

@reslear Hey, thanks! We don't test unplugin-vue-router / vue-router/auto-routes directly, but @ionic/vue-router consumes the standard vue-router 5 Router instance, so file-based route generation should behave the same as it does with plain vue-router 5, I'd expect.

Copy link
Copy Markdown
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

All my comments are suggestions and not blockers. I found no issues during the manual testing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to consider making a global file for the packages. That way all the frameworks are consistent with their testing. We can always override them per framework as needed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's probably a good idea, but outside of the scope of this PR for sure

) {
(window as any).__ionSawInvisible = true;
}
if (m.type === 'childList') {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be beneficial to check if __ionSawInvisible is already true. I don't see the need to check the children if that's the case.

Suggested change
if (m.type === 'childList') {
if (!(window as any).__ionSawInvisible && m.type === 'childList') {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export async function waitForAnimationsComplete(page: Page, selector: string): Promise<void> {
await page.evaluate(async (sel: string) => {
const el = document.querySelector(sel);
if (!el) return;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function doesn't throw an error like the other 2. Is that intentional? It could be helpful to know that the element didn't exist.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +15 to +18
* Ports the unit tests in packages/vue-router/__tests__/locationHistory.spec.ts
* to user-observable Playwright scenarios. Each describe block corresponds to
* one Jest case so the mapping is auditable when removing the Jest suite.
*
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we need this commented into the code. A comment on GitHub should be enough to let the reviewer know what changed.

Suggested change
* Ports the unit tests in packages/vue-router/__tests__/locationHistory.spec.ts
* to user-observable Playwright scenarios. Each describe block corresponds to
* one Jest case so the mapping is auditable when removing the Jest suite.
*

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was mostly for the future if I left the Jest tests in. I ultimately did not.
f898584

*/

test.describe('Location History', () => {
// Maps to: 'should correctly add an item to location history'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related to my comment above. These comments can be left in GitHub but doesn't seem beneficial to keep in code.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name: 'Mobile Chrome',
use: {
browserName: 'chromium',
...devices['Pixel 5'],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this one testing a phone while Angular tests a desktop?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Swipe-to-go-back is iOS-only and only fires on a touch viewport, so the gesture tests need a phone device profile. The Angular suite doesn't exercise gestures, so a desktop chrome viewport is fine there.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also doesn't make a lot of sense to me that we'd test on non-mobile views primarily considering most of our use case is mobile views

shell: bash
working-directory: ./packages/vue/test/build/${{ inputs.app }}
- name: 📦 Install Playwright Browsers
run: npx playwright install chromium
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's the case, can you either make a new PR or in this one to update the other frameworks to only use chromium. No need to inconsistent.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please mention how to use this in the Vue testing docs. Either in this PR or a new one, also add it for React Router.

Comment thread packages/vue/test/apps/vue3/package.json
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance we can get the mode switch like in React?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thetaPC thetaPC removed the request for review from gnbm May 7, 2026 18:12
@thetaPC thetaPC requested a review from brandyscarney May 7, 2026 18:12
Copy link
Copy Markdown
Member

@brandyscarney brandyscarney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found some issues while testing that I left as a comment on Jira. Great updates otherwise!

Comment thread BREAKING.md

The `@ionic/vue-router` package now requires Vue Router v5. Vue Router v4 is no longer supported. Vue Router v5 also raises its peer requirement on Vue itself, so the minimum supported Vue version moves to `3.5.0`.

**Minimum Version Requirements**
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also open a PR to update this on the docs site: https://ionicframework.com/docs/reference/support#ionic-vue

Side note: we should probably add columns for the supported router version as well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: core @ionic/core package package: vue @ionic/vue package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants